fix: correctly check equality for CaseInsensitiveMap #1235
Conversation
This comment has been minimized.
This comment has been minimized.
| override val entries: MutableSet<MutableMap.MutableEntry<String, Value>> | ||
| get() = impl.entries.map { | ||
| Entry(it.key.s, it.value) | ||
| Entry(it.key.normalized, it.value) | ||
| }.toMutableSet() | ||
|
|
||
| override val keys: MutableSet<String> | ||
| get() = impl.keys.map { it.s }.toMutableSet() | ||
| get() = impl.keys.map { it.normalized }.toMutableSet() |
There was a problem hiding this comment.
These changes mean that tests like the following will fail:
@Test
fun testSimpleKeyOp() {
val map = CaseInsensitiveMap<String>()
map["A"] = "apple"
assertTrue(map.keys.contains("A"))
}
Is that expected from a case insensitive map? I think the keys should remain unmodified, they should only be case-insensitive for equality operations.
Maybe both map.keys.contains("A") and map.keys.contains("a") should be true.
There was a problem hiding this comment.
Oh yes, they should both be true. Revision incoming...
Wait, maybe not. The return type of keys is MutableSet<String> which cannot comprehend case-insensitivity.
Actually that probably just means we're missing a CaseInsensitiveSet abstraction.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| override fun addAll(elements: Collection<String>) = | ||
| elements.fold(false) { modified, item -> add(item) || modified } |
There was a problem hiding this comment.
Could be simplified to elements.any { add(it) }, same applies for removeAll
There was a problem hiding this comment.
No, any short-circuits once it finds a match. If we used that, we'd only ever add at most a single item.
There was a problem hiding this comment.
Oh ok. I updated the implementation before posting this comment and all the tests still passed, but it's probably just the way they're written
| @@ -0,0 +1,11 @@ | |||
| package aws.smithy.kotlin.runtime.collections | |||
|
|
|||
| internal class CaseInsensitiveString(val original: String) { | |||
There was a problem hiding this comment.
Missing tests for CaseInsensitiveString. It is tested indirectly through the other classes though
There was a problem hiding this comment.
Tested indirectly but you're right, it'd be better to have dedicated tests. Will add!
This comment has been minimized.
This comment has been minimized.
1 similar comment
Affected ArtifactsChanged in size
|
2 participants
Issue #
(none)
Description of changes
Fix for #1233 (comment)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.